Skip to content

Glasgow | ITP May 2025 | Adiyah Farhan | Sprint 3 | Alarm Clock #741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AdiyahFarhan
Copy link

… acceptance criteria

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Implemented the setAlarm() function in alarm clock according to given acceptance criteria

Questions

No

@AdiyahFarhan AdiyahFarhan added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 2, 2025
@arun-john arun-john added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 5, 2025
@arun-john
Copy link

Hello Adiyah, good effort on this and nice to see the logic broken out into functions with good comments!

Did you try running the automated tests with npm test? I can see some failures here
I see you've tried to implement the color change / flashing logic from Extra Tasks. Color change seems to be working well, but probably needs a reset when starting the alarm again. Flashing logic needs more changes, in case you were planning to attempt that...

We are also missing some index.html changes (Pls. refer back to readme.md for details)

Good luck with the changes!

@AdiyahFarhan
Copy link
Author

Thank you, @arun-john, for taking the time to review my pull request. I have reviewed your feedback and updated the files accordingly. The test failure was due to a typo error, which I have now corrected. Please check. Thanks

@AdiyahFarhan AdiyahFarhan added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 8, 2025
@arun-john arun-john added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 8, 2025
@arun-john
Copy link

Great, seems all good!

One final thing - Try testing this out manually by setting the time to say 30 (and click Set Alarm). While the clock is counting down, set it to a different value, say 5 and click Set Alarm. Check what happens.

…ile running the interval. Now, the function will successfully update the interval according to new entered value
@AdiyahFarhan
Copy link
Author

Hi @arun-john, Thank you for bringing this issue to my attention. I have revised my function to address the problem. Please review the updated implementation.

@AdiyahFarhan AdiyahFarhan added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 9, 2025
@arun-john
Copy link

All working...good work on this Adiyah! 👍

@arun-john arun-john added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Volunteer to add when work is complete and all review comments have been addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants